Allow create PVPool instances in locations without PV #1215
Conversation
github-actions
bot
added
part:docs
There was a problem hiding this comment.
Pull Request Overview
This PR enables the creation of PVPool instances even in locations without any PV installations.
- Conditionally initializes the bounds_tracker attribute based on the presence of component_ids
- Adds a check in the stop method to safely await the stop of bounds_tracker only when it has been instantiated
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/frequenz/sdk/timeseries/pv_pool/_pv_pool_reference_store.py | Conditionally initialize and stop the bounds_tracker based on component_ids |
| RELEASE_NOTES.md | Added note regarding creation of PV Pool instances without PV |
| self.bounds_channel.new_sender(), | ||
| ) | ||
| self.bounds_tracker.start() | ||
|
|
There was a problem hiding this comment.
[nitpick] Consider adding an inline comment to explain why bounds_tracker is initialized to None and conditionally set based on component_ids. This will help maintainers understand the intent behind this change.
| # Initialize bounds_tracker to None. It will be conditionally assigned | |
| # if component_ids are provided or derived, as it is only needed to track | |
| # system bounds for the specified components. |
Without this, it was hard to create `PVPool`s at locations without PV. Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
| if self.bounds_tracker is not None: | ||
| await self.bounds_tracker.stop() |
There was a problem hiding this comment.
Not introduced by this PR, but just FYI, as I mentioned in other insteances and at least if this is a BackgroundService, stopping of sub-services should be done separately via cancel() and wait(), and leave stop() being just an alias for self.cancel(); await self.
There was a problem hiding this comment.
It is not a bug the way it is now, at least not in this layer, because this is in the *reference_store, which is supposed to be shared already.
But we will have to look at the outer layers, where we just removed the call to *reference_store.stop() as a temporary fix.
github-project-automation
bot
moved this from To do
to Review approved
in Python SDK Roadmap
github-project-automation
bot
moved this from Review approved
to Done
in Python SDK Roadmap
2 participants
Closes #1130